Conversation
Task Queue Isolation: - Make task queue name configurable via TASK_QUEUE_NAME env var - Prevents workers from stealing tasks meant for other services Reshare Improvements: - Improved logging with request_party_id and local_party_prefix fields - Fixed party ID generation to use service's own LocalPartyPrefix config Bug Fixes: - Fix policy ID generation: check uuid.Nil instead of empty string - Auth middleware: validate token if provided, even when auth disabled Documentation: - Add worker-config.json example - Add LOCAL_DEPLOYMENT.md cheatsheet 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
WalkthroughThe changes modify authentication middleware control flow, UUID validation in policy creation, environment-aware queue naming for tasks, local party ID generation in vault service, and introduce a new worker configuration file with runtime settings for connectivity and operational parameters. Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom Pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vault/service.go (1)
1-255: Address gofmt formatting issue.The CI pipeline is reporting that this file is not properly formatted. Please run
gofmt -s -w vault/service.goto fix the formatting issue before merging.#!/bin/bash # Check formatting of vault/service.go gofmt -s -l vault/service.go
🤖 Fix all issues with AI agents
In @.run/LOCAL_DEPLOYMENT.md:
- Line 12: Update the LOCAL_DEPLOYMENT.md to clarify that the DYLD_LIBRARY_PATH
step is macOS-specific by adding a brief platform note near the existing export
line referencing DYLD_LIBRARY_PATH (and the darwin include path) — mark the
prerequisite as “macOS only” and add a short sentence advising Linux/Windows
users to use their OS-specific library/PATH setup or ensure go-wrappers build
artifacts are on their system PATH instead.
- Line 12: The file currently hard-codes developer-specific paths into
DYLD_LIBRARY_PATH (e.g., "/Users/dev/dev/vultisig/...") which will break other
environments; update the doc to instruct users to set a project root environment
variable (suggest VULTISIG_HOME) and show how to export DYLD_LIBRARY_PATH using
that variable (reference DYLD_LIBRARY_PATH and VULTISIG_HOME), or alternatively
add a prerequisites note telling developers to adjust those paths to their local
clone and demonstrate the relative path approach (e.g., use
$VULTISIG_HOME/go-wrappers/includes/darwin or ./go-wrappers/includes/darwin)
instead of the hard-coded absolute paths on lines 12 and 17.
- Around line 81-91: Update the local deployment docs to document the
TASK_QUEUE_NAME env var when running the worker: explain how to set
TASK_QUEUE_NAME alongside VS_WORKER_CONFIG_NAME when launching
cmd/worker/main.go (e.g., TASK_QUEUE_NAME=verifier-queue
VS_WORKER_CONFIG_NAME=worker-config go run cmd/worker/main.go) and add a note
about single-worker setups stating whether the default queue name (if any) is
sufficient or when a custom TASK_QUEUE_NAME must be used to avoid cross-service
task stealing; reference the TASK_QUEUE_NAME variable and the
VS_WORKER_CONFIG_NAME/ cmd/worker/main.go invocation so readers know where to
apply it.
🧹 Nitpick comments (2)
worker-config.json (1)
1-34: LGTM: Configuration file for local development.The configuration structure is appropriate for local development with sensible defaults. The static analysis warning about credentials in line 23 is a false positive—these are clearly example credentials for local development (myuser/mypassword, minioadmin/minioadmin).
To prevent accidental production use, consider adding a comment at the top of the file indicating this is for local development only.
📝 Optional: Add documentation header
{ + "_comment": "This configuration is for local development only. Do not use in production.", "log_format": "text", "vault_service": {plugin/tasks/tasks.go (1)
10-19: LGTM: Task queue isolation enabled.The environment-based queue name configuration successfully enables running multiple workers with isolated task queues, which aligns with the PR objectives. The implementation:
- Reads
TASK_QUEUE_NAMEenvironment variable for flexibility- Falls back to
defaultQueueNamefor backward compatibility- Is determined at initialization time
One minor consideration: Since
QUEUE_NAMEis an exported variable rather than a constant, it's technically modifiable at runtime (though unlikely in practice). If immutability is important, consider making it a function or using an unexported variable with an exported getter.Based on learnings, this queue name is correctly separated from infrastructure references like database/bucket names.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.run/LOCAL_DEPLOYMENT.mdinternal/api/middleware.gointernal/api/policy.goplugin/tasks/tasks.govault/service.goworker-config.json
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: evgensheff
Repo: vultisig/verifier PR: 286
File: plugin/tasks/tasks.go:9-9
Timestamp: 2025-07-31T09:12:18.809Z
Learning: In the Vultisig verifier project, "vultisig-verifier" appears in configuration files, docker-compose.yaml, and test files as infrastructure references (PostgreSQL database names, S3/MinIO bucket names, Docker environment variables) and should not be confused with the QUEUE_NAME constant in plugin/tasks/tasks.go which specifically refers to the Asynq task queue name. These infrastructure references should remain unchanged when queue names are modified.
📚 Learning: 2025-05-26T00:35:50.407Z
Learnt from: RaghavSood
Repo: vultisig/verifier PR: 87
File: internal/api/plugin.go:0-0
Timestamp: 2025-05-26T00:35:50.407Z
Learning: Policy verification logic in internal/api/plugin.go will be refactored when recipe workflows are applied to policy verification in a subsequent PR.
Applied to files:
internal/api/policy.go
📚 Learning: 2025-07-24T11:10:49.405Z
Learnt from: garry-sharp
Repo: vultisig/verifier PR: 274
File: internal/storage/postgres/policy.go:304-304
Timestamp: 2025-07-24T11:10:49.405Z
Learning: In internal/storage/postgres/policy.go, the DeletePluginPolicySync method intentionally updates the plugin_policies table to set deleted=true rather than deleting from plugin_policy_sync table. This is the correct behavior as confirmed by garry-sharp in PR #274.
Applied to files:
internal/api/policy.go
📚 Learning: 2025-06-10T18:57:04.359Z
Learnt from: webpiratt
Repo: vultisig/verifier PR: 156
File: internal/api/plugin.go:72-86
Timestamp: 2025-06-10T18:57:04.359Z
Learning: In internal/api/plugin.go (SignPluginMessages), the correct workflow is to persist the transaction via txIndexerService.CreateTx *before* policy evaluation, leaving it in the PROPOSED state; this behavior is intentional and should not be treated as an issue.
Applied to files:
internal/api/policy.go
📚 Learning: 2025-07-09T22:25:01.536Z
Learnt from: webpiratt
Repo: vultisig/verifier PR: 240
File: internal/api/policy.go:72-96
Timestamp: 2025-07-09T22:25:01.536Z
Learning: In internal/api/policy.go, the commented-out block for mocking required fields (lines 72-96) is intentionally kept as temporary code until UI fixes are completed, as confirmed by webpiratt. This pattern is acceptable in this specific context.
Applied to files:
internal/api/policy.go
📚 Learning: 2025-07-31T09:12:18.809Z
Learnt from: evgensheff
Repo: vultisig/verifier PR: 286
File: plugin/tasks/tasks.go:9-9
Timestamp: 2025-07-31T09:12:18.809Z
Learning: In the Vultisig verifier project, "vultisig-verifier" appears in configuration files, docker-compose.yaml, and test files as infrastructure references (PostgreSQL database names, S3/MinIO bucket names, Docker environment variables) and should not be confused with the QUEUE_NAME constant in plugin/tasks/tasks.go which specifically refers to the Asynq task queue name. These infrastructure references should remain unchanged when queue names are modified.
Applied to files:
plugin/tasks/tasks.go
🧬 Code graph analysis (1)
internal/api/middleware.go (1)
plugin/server/auth.go (1)
Auth(11-13)
🪛 Checkov (3.2.334)
worker-config.json
[medium] 23-24: Basic Auth Credentials
(CKV_SECRET_4)
🪛 GitHub Actions: Deploy Verifier Server
vault/service.go
[error] 1-1: Code is not formatted properly: gofmt -s -l . reported unformatted file(s). Run 'gofmt -s -w' to fix.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (5)
internal/api/policy.go (1)
71-73: LGTM: Improved UUID validation.Using
uuid.Nilfor checking unset UUIDs is more idiomatic and efficient than converting to string and comparing to empty string. This change improves code clarity and correctness.internal/api/middleware.go (1)
41-47: LGTM: Security improvement for auth validation.The updated logic ensures that when a token is provided, it's always validated even when authentication is disabled. This prevents potential security bypasses where invalid tokens could be accepted simply because auth is disabled. The behavior is now:
- No token + auth disabled → proceed
- No token + auth enabled → 401
- Token provided → always validate (regardless of auth setting)
This aligns with the security fix mentioned in the PR objectives.
vault/service.go (2)
211-217: LGTM: Improved logging clarity.The renamed logging field from "local_party_id" to "request_party_id" and the addition of "local_party_prefix" make it clearer which party ID is from the request versus the service's own configuration. This improves observability for reshare operations.
227-235: LGTM: Correct party ID generation using service's own prefix.The fix ensures the service generates its local party ID using its own configured
LocalPartyPrefixrather than accepting a prefix from the request. This is critical for proper task queue isolation and prevents clients from controlling the service's party identification.The comment clearly explains the rationale for this change.
plugin/tasks/tasks.go (1)
30-30: Correct usage of dynamic queue name.The
GetTaskResultfunction correctly uses the dynamically determinedQUEUE_NAMEvariable, ensuring it queries the appropriate queue based on the environment configuration.
.run/LOCAL_DEPLOYMENT.md
Outdated
| docker network create shared_network | ||
|
|
||
| # Set library path (required for TSS operations) | ||
| export DYLD_LIBRARY_PATH=/Users/dev/dev/vultisig/go-wrappers/includes/darwin/:$DYLD_LIBRARY_PATH |
There was a problem hiding this comment.
Clarify that DYLD_LIBRARY_PATH setup is macOS-only.
The DYLD_LIBRARY_PATH environment variable on Line 12 is specific to macOS. Linux and Windows developers will not need this step. Add a platform note to prevent confusion.
🔎 Suggested clarification
# Prerequisites (macOS only)
```bash
# macOS: Set library path (required for TSS operations)
export DYLD_LIBRARY_PATH=/Users/dev/dev/vultisig/go-wrappers/includes/darwin/:$DYLD_LIBRARY_PATHFor Linux and Windows, ensure the go-wrappers build artifacts are in your system PATH or library path as appropriate for your OS.
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @.run/LOCAL_DEPLOYMENT.md at line 12, Update the LOCAL_DEPLOYMENT.md to
clarify that the DYLD_LIBRARY_PATH step is macOS-specific by adding a brief
platform note near the existing export line referencing DYLD_LIBRARY_PATH (and
the darwin include path) — mark the prerequisite as “macOS only” and add a short
sentence advising Linux/Windows users to use their OS-specific library/PATH
setup or ensure go-wrappers build artifacts are on their system PATH instead.
</details>
<!-- fingerprinting:phantom:triton:mongoose -->
---
_⚠️ Potential issue_ | _🟡 Minor_
**Replace hard-coded developer paths with instructions for setup.**
Lines 12 and 17 contain hard-coded paths (`/Users/dev/dev/vultisig/...`) that are specific to one developer's machine. This will require manual edits for other developers. Provide instructions to set this up generically, for example via an environment variable or by describing the relative directory structure.
<details>
<summary>🔎 Suggested approach</summary>
```bash
# Example: guide users to set VULTISIG_HOME or similar
export VULTISIG_HOME=~/path/to/vultisig # or wherever they cloned the repos
export DYLD_LIBRARY_PATH=$VULTISIG_HOME/go-wrappers/includes/darwin:$DYLD_LIBRARY_PATH
cd $VULTISIG_HOME/verifier
Alternatively, note in the prerequisites section that developers must adjust paths for their environment.
Also applies to: 17-17
🤖 Prompt for AI Agents
In @.run/LOCAL_DEPLOYMENT.md at line 12, The file currently hard-codes
developer-specific paths into DYLD_LIBRARY_PATH (e.g.,
"/Users/dev/dev/vultisig/...") which will break other environments; update the
doc to instruct users to set a project root environment variable (suggest
VULTISIG_HOME) and show how to export DYLD_LIBRARY_PATH using that variable
(reference DYLD_LIBRARY_PATH and VULTISIG_HOME), or alternatively add a
prerequisites note telling developers to adjust those paths to their local clone
and demonstrate the relative path approach (e.g., use
$VULTISIG_HOME/go-wrappers/includes/darwin or ./go-wrappers/includes/darwin)
instead of the hard-coded absolute paths on lines 12 and 17.
- Clarify DYLD_LIBRARY_PATH is macOS-only - Add TASK_QUEUE_NAME documentation for multi-worker setups 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Info already exists in devenv/README.md and CLAUDE.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
1 participant
Summary
Core improvements extracted from local development work. No devctl/devenv CLI - those live in vultisig-cluster.
Task Queue Isolation
TASK_QUEUE_NAMEenv varReshare Improvements
request_party_idandlocal_party_prefixfieldsLocalPartyPrefixconfig (not the one from request)Bug Fixes
uuid.Nilinstead of empty stringDocumentation
worker-config.jsonexample for local developmentLOCAL_DEPLOYMENT.mdcheatsheetTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.